tests/json_rpc_e2e.rs (1)

1030-1033: ⚡ Quick win

Assert the JSON-RPC error.message field directly

Using error.to_string().contains("mon_missing") is brittle to envelope/serialization changes. Assert against error["message"] to pin the transport contract explicitly.

Suggested change

-    assert!(
-        error.to_string().contains("mon_missing"),
-        "missing monitor error should name id: {error}"
-    );
+    let msg = error
+        .get("message")
+        .and_then(Value::as_str)
+        .unwrap_or_default();
+    assert!(
+        msg.contains("mon_missing"),
+        "missing monitor error should name id in error.message: {error}"
+    );

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/json_rpc_e2e.rs` around lines 1030 - 1033, The current assertion checks
the error string via error.to_string(), which is brittle; instead index into the
JSON-RPC error object and assert on its "message" field directly (e.g., use
error["message"] or error.get("message") on the serde_json::Value named error)
to verify it contains or equals "mon_missing" so the transport contract is
pinned; update the assert!(...) that references error.to_string() to check the
JSON field of error (and adjust to handle Option/Type accordingly).

src/openhuman/monitor/runner.rs (1)

114-195: 💤 Low value

Reader tasks are not joined after child process exits.

After child.wait() completes and remaining lines are drained via try_recv, the spawned reader tasks (spawn_reader) are left detached. While they will eventually terminate when the pipe handles are closed, there's a brief window where they could still be trying to send on the closed channel.

This is unlikely to cause issues since tx.send() returns an error when the receiver is dropped (which is handled), but for completeness you could store the JoinHandles and await them after draining.

This is a minor robustness concern - the current implementation is functionally correct.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/monitor/runner.rs` around lines 114 - 195, The reader tasks
spawned by spawn_reader in run_child are not awaited after child.wait(), leaving
detached JoinHandles that may race sending to the channel; capture the
JoinHandle returned by each spawn_reader call (e.g. let stdout_handle =
spawn_reader(...), let stderr_handle = spawn_reader(...)), drop or close the
sender (line_tx) after draining remaining messages, then await those handles
(stdout_handle.await and stderr_handle.await) before returning from the
child.wait() branch so the reader tasks are properly joined and any errors are
observed.

src/openhuman/monitor/types.rs (1)

89-119: 💤 Low value

Inconsistent serde field naming between response types.

MonitorStartResponse, MonitorStopResponse, and MonitorReadResponse use camelCase renames (monitorId, outputFile) while MonitorSnapshot (used in MonitorListResponse) uses snake_case. This creates an inconsistent API surface where monitor_list returns snake_case fields but other endpoints return camelCase for the same conceptual fields.

Consider unifying: either apply #[serde(rename_all = "camelCase")] to all response types or keep everything snake_case.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/monitor/types.rs` around lines 89 - 119, The issue is
inconsistent serde naming across response types; unify to camelCase by applying
#[serde(rename_all = "camelCase")] to MonitorSnapshot and MonitorListResponse
(and any other structs in this module lacking it) so field names like
monitor_id/output_file are serialized the same as MonitorStartResponse,
MonitorStopResponse, and MonitorReadResponse; update or remove any per-field
#[serde(rename = "...")] annotations as needed so all structs
(MonitorStartResponse, MonitorStopResponse, MonitorReadResponse,
MonitorSnapshot, MonitorListResponse) consistently use camelCase.

src/openhuman/monitor/tools.rs (1)

61-71: ⚡ Quick win

Centralize monitor command classification to prevent gate drift.

This classification/escalation block duplicates src/openhuman/monitor/ops.rs (start), which can desync approval gating (external_effect_with_args) from execute-time enforcement over time. Extract a shared helper and reuse it in both places.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/monitor/tools.rs` around lines 61 - 71, The command
classification/escalation logic in external_effect_with_args (using
security.classify_command and optional SecurityPolicy::parse_declared_class then
security.gate_decision) is duplicated in src/openhuman/monitor/ops.rs (start);
extract that logic into a single helper (e.g., classify_and_escalate_command or
classify_command_with_declared) in monitor::tools, have the helper accept
(&self, command: &str, declared_category: Option<&str>) and return the final
GateDecision or resulting Class, then replace the inline code in
external_effect_with_args and the start function in ops.rs to call the new
helper so both use the same classification/escalation path.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes